Add RestartActorException #1169
Conversation
ela-kotulska-frequenz
added
the
part:actor
github-actions
bot
added
part:docs
Actor should be able to restart `_run` method for example to update formula and start listening on new receiver. Restarting actor should not print exception. Signed-off-by: Elzbieta Kotulska <[email protected]>
ela-kotulska-frequenz
force-pushed
the
restart_actor
branch
from
5009232 to
304035c
Compare
Because this nane is more descriptive. Signed-off-by: Elzbieta Kotulska <[email protected]>
ela-kotulska-frequenz
force-pushed
the
restart_actor
branch
from
6d67fb4 to
b54d916
Compare
There was a problem hiding this comment.
I understand where this comes from, but I'm not sure if it is the path we want to follow in the future (#632).
That said, this is how things work now and I can see how having a convenient way to restart is helpful, but an exception is also not the only way to restart, and have some implications. Mainly, anyone, at any level in the call stack, can trigger an actor restart.
An alternative that can prevent this, would be to use a asyncio.Event and await the event or the actor to finish, and then restart it if we got the event. Then adding the Actor class a restart() method that just triggers the event.
Even when Python already uses this exception mechanism for exits (like SystemExit or GeneratorExit), I'm not sure this is the right approach, because in those cases they are BaseExceptions and nobody is supposed to catch them (and body will, unless they explicitly catch BaseException),but with RestartActorException, it shouldn't inherit from BaseException (as per Python docs), but if it doesn't, it also mean anyone using except Exception, will probably avoid restarting the actor (unwillingly), so it has its risks.
All that said, since restarting actors like this should probably change in the future, I'm not sure how important is to get this perfectly right, but unless we really need the exception, I think it would be safer to go for a restart() method with an event.
| Args: | ||
| iteration: The current iteration of the restart. | ||
| """ | ||
| async def _delay(self) -> None: |
There was a problem hiding this comment.
Maybe make it a bit more explicit?
| async def _delay(self) -> None: | |
| async def _delay_restart(self) -> None: |
|
Closed, to discuss later how we should implement it |
2 participants
Actor should be able to restart
_runmethodFor example to update formula and start listening on new receiver.
Restarting actor should not print exception.